add render-time and async node error recovery pattern#802
add render-time and async node error recovery pattern#802tmarmer wants to merge 36 commits intoerror-controllerfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## error-controller #802 +/- ##
====================================================
+ Coverage 86.02% 86.36% +0.33%
====================================================
Files 513 512 -1
Lines 23566 24020 +454
Branches 2703 2828 +125
====================================================
+ Hits 20272 20744 +472
+ Misses 2956 2942 -14
+ Partials 338 334 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 125.88kB (2.17%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: plugins/check-path/coreAssets Changed:
view changes for bundle: plugins/metrics/coreAssets Changed:
view changes for bundle: react/subscribeAssets Changed:
view changes for bundle: plugins/beacon/coreAssets Changed:
view changes for bundle: plugins/markdown/coreAssets Changed:
view changes for bundle: react/playerAssets Changed:
view changes for bundle: core/playerAssets Changed:
view changes for bundle: plugins/stage-revert-data/coreAssets Changed:
view changes for bundle: plugins/reference-assets/coreAssets Changed:
view changes for bundle: plugins/async-node/coreAssets Changed:
view changes for bundle: plugins/common-expressions/coreAssets Changed:
view changes for bundle: plugins/common-types/coreAssets Changed:
|
|
/canary |
| /** Returns a function to be used as the `replacer` for JSON.stringify that tracks and ignores circular references. */ | ||
| const makeJsonStringifyReplacer = (): ReplacerFunction => { | ||
| const cache = new Set(); | ||
| return (_: string, value: any) => { | ||
| if (typeof value === "object" && value !== null) { | ||
| if (cache.has(value)) { | ||
| // Circular reference found, discard key | ||
| return "[CIRCULAR]"; | ||
| } | ||
| // Store value in our collection | ||
| cache.add(value); | ||
| } | ||
| return value; | ||
| }; | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why do we need to do this? Shouldn't internal Player state be serializable?
There was a problem hiding this comment.
The issue here stems from the ResolverError adding in the failed node in its metadata. We need a better solution but this was put in place to allow the metadata to get logged without throwing errors since the node itself has circular references.
If we don't want the node logged or set in the dataController as part of captureError we either need to not include the metadata in logs/data or we need another mechanism to identify resolver nodes with to avoid sending the whole object to keep its reference.
There was a problem hiding this comment.
So this is so that the error includes the originating AST node, gotcha. I think a reference should be able to be stored fine in the Data Controller without serialization right? For logging can we just reference the asset ID?
There was a problem hiding this comment.
If the error controller was aware of the metadata being logged we could, but because any error with any metadata could pass through it, we can't identify when we're getting something like a Node or a simpler object. That and sometimes the originating Node being logged might be an async node or something else that generated the asset.
| export const ResolverStages = { | ||
| ResolveOptions: "resolve-options", | ||
| SkipResolve: "skip-resolve", | ||
| BeforeResolve: "before-resolve", | ||
| Resolve: "resolve", | ||
| AfterResolve: "after-resolve", | ||
| AfterNodeUpdate: "after-node-update", | ||
| } as const; |
There was a problem hiding this comment.
Is there a way we can automate this?
There was a problem hiding this comment.
Couldn't find a way to automatically form this object, but simplified the types here to use an enum. If you have any ideas here I'm open to try something else
…if no transition available.
core/player/src/controllers/error/utils/__tests__/isErrorWithMetadata.test.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
…etadata.test.ts Co-authored-by: Ketan Reddy <KetanReddy@users.noreply.github.com>
… of async nodes on error
…ed across the flow instance and error controller
KVSRoyal
left a comment
There was a problem hiding this comment.
iOS stuff lgtm---can you just double-check that everything public actually needs to be?
| override val type: String | ||
| get() = ErrorTypes.RENDER | ||
| override val severity: ErrorSeverity? | ||
| get() = ErrorSeverity.ERROR | ||
| override val metadata: Map<String, Any?>? | ||
| get() = mapOf( | ||
| "assetId" to rootAsset.asset.id, | ||
| ) |
There was a problem hiding this comment.
Since these are static, just assign them:
| override val type: String | |
| get() = ErrorTypes.RENDER | |
| override val severity: ErrorSeverity? | |
| get() = ErrorSeverity.ERROR | |
| override val metadata: Map<String, Any?>? | |
| get() = mapOf( | |
| "assetId" to rootAsset.asset.id, | |
| ) | |
| override val type: String = ErrorTypes.RENDER | |
| override val severity: ErrorSeverity? = ErrorSeverity.ERROR | |
| override val metadata: Map<String, Any?> = mapOf( | |
| "assetId" to rootAsset.asset.id, | |
| ) |
| cause: Throwable? = node.getSerializable("innerErrors", ListSerializer(ThrowableSerializer()))?.first(), | ||
| override val type: String = "", | ||
| override val severity: ErrorSeverity?, | ||
| override val metadata: Map<String, Any?>?, |
There was a problem hiding this comment.
I don't think these belong on the base JSErrorException type, as that is supposed to mirror the base Error TS class. We probably want a subclass that matches the PlayerErrorMetadata one
| * Represents a Player error with metadata | ||
| */ | ||
| @Serializable(with = PlayerErrorInfo.Serializer::class) | ||
| public class PlayerErrorInfo internal constructor( |
There was a problem hiding this comment.
Should this whole class just be a PlayerException (with metadata) instance instead of a NodeWrapper? It could then just use the ThrowableSerializer instead of a whole new construct
There was a problem hiding this comment.
I believe this type is meant to mimic this TS type, which is not an error itself. Maybe a rename would be helpful for clarity?
@cehan-Chloe any thoughts here?
There was a problem hiding this comment.
+1 this mimic the core TS type. There is already a PlayerError in iOS and I would like to use same name in mobile to keep the consistency hence the renaming. agree it's kinda confusing maybe we can rename the PlayerError in iOS and use PlayerError in all platform if it's more clear.
| coroutineExceptionHandler = | ||
| config.coroutineExceptionHandler ?: CoroutineExceptionHandler { _, throwable -> | ||
| if (state !is ReleasedState) { | ||
| logger.error("[HeadlessPlayer]: Error has been found") |
There was a problem hiding this comment.
Consolidate the two logs now — just pull the other one up if you think we should log it every time regardless of whether we can forward it to captureError.
There was a problem hiding this comment.
Oh my bad, had some extra logs when testing and forgot to remove this >.<
| override val type: String | ||
| get() = "TestError" | ||
| override val severity: ErrorSeverity? | ||
| get() = ErrorSeverity.ERROR | ||
| override val metadata: Map<String, Any?>? | ||
| get() = mapOf( | ||
| "testProperty" to "testValue", | ||
| ) |
| public fun setupPlayer( | ||
| plugins: List<Plugin> = this.plugins, | ||
| runtime: Runtime<*> = this.runtime, | ||
| includeThisInPlugins: Boolean = true, |
There was a problem hiding this comment.
Rather than an extra param, should we just do this automatically if this isn't contained in plugins?
| val refPlugin = ReferenceAssetsPlugin() | ||
| refPlugin.apply(runtime) | ||
| if (player is HeadlessPlayer) { | ||
| val invokable = (player as HeadlessPlayer).node.getInvokable<Unit>("registerPlugin") |
There was a problem hiding this comment.
We should just implement registerPlugin properly than hack it into a test.
There was a problem hiding this comment.
Agreed, but I think I want to avoid that here for the sake of not adding more scope to this PR. This was a hack anyway to get around some issues the tests had with the ReferenceAssetsPlugin. I made some changes earlier that make it so this hack isn't needed and missed updating the tests. For now, I've removed any resgisterPlugin code from the tests.
Ideally I would like to remove that dependency on another plugin to test the AsyncNodePlugin but it seems like there is no easy way to decouple it in these tests so I might look to do that later as well.
| } | ||
|
|
||
| val errorMessage = assertThrows<Exception> { | ||
| val refPlugin = ReferenceAssetsPlugin() |
There was a problem hiding this comment.
Isn't this already applied on the Player instance?
nancywu1
left a comment
There was a problem hiding this comment.
iOS changes looks good, do we have any examples/mocks in the demo app with errorTransitions or error state structure, if not maybe can add them in later?
captureErrorto fail the player state if no error transition is available.PlayerViewModel.failwhich now uses the error controller.PlayerErrorMetadatainterface for errors to implement to include info required for the error controller to capture errorsChange Type (required)
Indicate the type of change your pull request is:
patchminormajorN/ADoes your PR have any documentation updates?
Release Notes
AsyncNodePlugin'sonAsyncNodeErrorhook to depend on theErrorController'sonErrorhook for finding errors.ErrorController.captureErrorand allow for error recovery on all platforms.PlayerErrorMetadatainterface across platforms and ensure serialization of errors keeps additional metadata.ErrorController.captureErrorwill prefer data from the error object when calling theonErrorhook.📦 Published PR as canary version:
0.15.1--canary.802.31569Try this version out locally by upgrading relevant packages to 0.15.1--canary.802.31569